Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added PersisterFactory to ORM. #1260

Closed
wants to merge 1 commit into from
Closed

Conversation

guilhermeblanco
Copy link
Member

No description provided.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3507

We use Jira to track the state of pull requests and the versions they got
included in.

*
* @return \Doctrine\ORM\Persisters\PersisterFactory
*/
public function getPersisterFactory()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method doesn't seem to fit the EntityManager at all: it would honestly be better to not expose it to end-users like this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it seems the wrong way to expose it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hibernate exposes it at EntityManager level because it can be swappable with another implementation.
Default implementation is also reachable from outside, allowing you to provide a ServiceInjector (read as DI Container).
I'm ok to move this to UnitOfWork, but I would not consider this as an extension point (the part I wanted to discuss internally). This means there'll be no interface bound to PersisterFactory.
Right now I made it part of EntityManager and wanted to discuss IF we'd make it an extension point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR: the UnitOfWork as-is is already poop: let's stash poop in there instead of shoveling it on other classes.

That makes our EntityManager some kind of locator (meh).
I'd prefer to make a tradeoff here, and do this crappy thing in the UnitOfWork instead (yes, with all the deficiencies that come from it).

The UnitOfWork as we know it will go away at some point in time, while the EntityManager will stay and be consumed by userland.

@Ocramius Ocramius changed the title Added PersisterFactory to ORM. [WIP] Added PersisterFactory to ORM. Jan 16, 2015
@Ocramius
Copy link
Member

This change seems wrong overall: the EntityManager seems to be the wrong place where to stash away the persister factory, and the interface change is not acceptable.

*
* @package Doctrine\ORM\Persisters
*/
class PersisterFactory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be an interface with a default implementation to make it an extension point ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree: introducing a concrete implementation with no interface seems a bit messy here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if we consider this as an extension point, which in the current state of this PR it isn't.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with guilherme, it is not an extension point at the moment.

@beberlei
Copy link
Member

General question, whats the benefit of this? Do we need htis refactoring? What for?

@guilhermeblanco
Copy link
Member Author

@beberlei AS currently it is here, 0 benefits.
Problem comes when you want to have extra lazy many to many criteria. It is required to share same SQL alias generation between persisters. I thought about first creating a PersisterFactory that could eliminate chunks of UnitOfWork code, then sharing as a setter injection some sort of SQL generation helper between them. That would then remove large chunks of code inside Persisters.
This PR is trying to do the first initial step. I wanted to break down into multiple PRs because it becomes easier to review.

@guilhermeblanco
Copy link
Member Author

I'll close this PR soon. Will implement a new one using PersisterFactory on UnitOfWork.

@Ocramius Ocramius changed the title [WIP] Added PersisterFactory to ORM. Added PersisterFactory to ORM. Jan 24, 2015
@Ocramius
Copy link
Member

Closing as won't fix - this is interesting for 3.x, but not as-is (too many conflicts)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants